-
Notifications
You must be signed in to change notification settings - Fork 21.4k
eth/fetcher: remove alternates, announced maps #32773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@healthykim So we change how we would operate with many peers, right? I was running nodes like that, and would like to keep that possible. Maybe the change in load is marginal, just want to double-check. |
txFetcherWaitingPeers = metrics.NewRegisteredGauge("eth/fetcher/transaction/waiting/peers", nil) | ||
txFetcherWaitingHashes = metrics.NewRegisteredGauge("eth/fetcher/transaction/waiting/hashes", nil) | ||
txFetcherQueueingPeers = metrics.NewRegisteredGauge("eth/fetcher/transaction/queueing/peers", nil) | ||
txFetcherQueueingHashes = metrics.NewRegisteredGauge("eth/fetcher/transaction/queueing/hashes", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to keep this metric somehow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the current change resolves this comment but it was not. I made this unresolved again. I'll take a look
eth/fetcher/tx_fetcher.go
Outdated
} | ||
} | ||
|
||
func (f *TxFetcher) announced(hash common.Hash) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We rely on the announces map[string]map[common.Hash]*txMetadataWithSeq
for determining whether the tx is announced or not.
You are right that the map iteration should be fast enough, especially the size of peer set is very limited. But we should also consider that this function will be invoked for every tx announcement.
However, another downside is we lose the ability to know how many transactions are in the announced status.
An alternative approach will be maintaining the map like
announced map[common.Hash]struct{}
.
This PR removes the
alternates
andannounced
maps intx_fetcher
.alternates
map is used only to check whether a transaction is in stage 2 (queued), and theannounced
map is used only to check whether a transaction is in stage 3 (fetching).announces
map). So maintaining two separate maps seems unnecessary.announces
map, which is shared across stage 2 and stage 3, to perform the same check.Given that the peer set is usually small (e.g., 50 by default), this iteration is expected to be fast.